-
Notifications
You must be signed in to change notification settings - Fork 82
Basic Virtual Memory Implementation Fixes & Improvements #165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Notice that address sanitizer is failing in CI. |
src/gui/windows/tlb/tlbview.h
Outdated
| void tlb_update(unsigned way, unsigned set, bool valid, unsigned asid, quint64 vpn, quint64 phys, bool write); | ||
|
|
||
| private: | ||
| const machine::TLB *tlb; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who owns this pointer?
src/machine/memory/tlb/tlb.h
Outdated
| #include <cstdint> | ||
|
|
||
| namespace machine { | ||
| enum TLBType { PROGRAM, DATA }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does LTB need to know this?
|
@jdupak thanks for review of interfacing to the memory model architecture. |
|
From my side, the changes to the processor pipeline diagram has been applied directly to the SVG files (src/gui/windows/coreview/schemas), but current design uses DRAW.IO source (extras/core_graphics) as the authoritative source of the pipeline visualization and SVGs are generated from this file. So the commit with SVG change should include |
|
For memory view, I would not complicate it with |
0bb04d1 to
ca4300b
Compare
This resolves the incorrect zoom behavior.
Add tracking of the hart's current privilege level to the core state so code handling exceptions/returns and visualization can read/update it from the central CoreState structure.
Add supervisor CSRs (sstatus, stvec, sscratch, sepc, scause, stval, satp) and a write handler that presents sstatus as a masked view of mstatus so supervisor-visible bits stay in sync.
ca4300b to
a6cbf71
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a typo in the dir name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no cmake logic to run these tests. I think we want to run them as cli tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the comment. I’ve added the CMake logic to run these as CLI tests in the commit 7a204cf.
Add privilege level mapping to the GUI so the current hart privilege (UNPRIV, SUPERV, HYPERV, MACHINE) is displayed in core state visualization.
Extend NewDialog with controls for virtual memory setup, including TLB number of sets, associativity, and replacement policy.
Introduce new components for displaying and tracking TLB state similar to cache. TLBViewBlock and TLBAddressBlock render per-set and per-way TLB contents, updated on tlb_update signals. TLBViewScene assembles these views based on associativity. TLBDock integrates into the GUI, showing hit/miss counts, memory accesses, stall cycles, hit rate, and speed improvement, with live updates from the TLB.
Introduce an "As CPU (VMA)" access option in the cached access selector to render memory contents as observed by the CPU through the frontend interface.
a6cbf71 to
bc32933
Compare
|
I pushed some slight edits. Barring the issue with new tests not being run I am fine with merging this. |
|
I am going through the code. I have one overall remark, that there are lot of formatting changes included in functional changes. I am not reluctant to formatting changes even that I think that sometimes formatting left by human to align for example some case lines into columns etc. has some value. But formatting changes unrelated to the functional changes make review harder. So I would keep with patches as they are but I would suggest to separate formatting, even over all later modified files in series, separate from functional changes. |
|
I am do not like As for enabling cache for accesses there is a hack in the QtRvSim which enforces next uncached region In the longer term, cacheability should be controlled from page tables. But
But the physical region marked to skip caching in cache implementation (current state) should be enough for now. |
|
Not so critical for now, but should be solved in the longer time perspective. The behavior of |
|
It seems that TLBs are updated from the start of the system. The TLB and its updates should be enable only when root register is set. And they should not be updated in |
|
There is one actual issue from CI: you cannot use ftruncate. It fails compilation on Win. |
Never mind, this is broken on master. I will fix that. It does not block this PR. |
Add a set of small assembly tests that exercise the SV32 page-table walker, SATP enablement and the new TLB code. The tests create a root page table and map a virtual page at 0xC4000000, then exercise several scenarios. The tests verify page-table walker behaviour, SATP switching and TLB caching/flush logic. Tests were written based on the consultation.
7a204cf to
b8728d1
Compare
Restructure the virtual memory test directory to align with the rest of the tests, and add CMake logic for running them.
b8728d1 to
0ae766f
Compare
Ensure that TLBs are only updated when the root register is set, and disable TLB updates while running in Machine mode.
c846a9e to
442a091
Compare
Decode MRET/SRET/URET in the decode stage, carry the return type through interstage registers, and pass it to ControlState::exception_return in the memory stage. Mark returns illegal if they request a higher privilege (e.g., MRET in S-mode). When returning from M to a less-privileged mode, clear MPRV per the privileged spec.

This PR addresses some previous feedback, most notably:
Tests:
UI Components: